Replace Report_AddComment with AddComment (Part 2) #9350

marcaaron · 2022-06-08T00:04:58Z

Details

This PR:

Remove the reportComment Pusher subscriptions (updateReportWithNewAction() remains and is used in the Urban Airship notification as I'm not sure how we are handling "onyx updates" there yet)
Updates DeprecatedAPI.Report_AddComment() to API.write('AddComment')

To minimize changes I decided to keep the "attachment" and "comment" flows together for now.

Test in connection with: https://github.com/Expensify/Web-Expensify/pull/33951

Fixed Issues (Related to

https://github.com/Expensify/Expensify/issues/211241

Tests

Same as QA but with an additional test to verify that users blocked by Concierge will see an error message inline.

Test Blocked from Concierge

Log in with a test user account
Begin chatting with Concierge
Sign into OldDot with your local expensify.com account
Block the test user via Concierge
Send a message from the user account to Concierge DM
Verify an inline message appears and the chat composer is disabled

QA Steps

Testing a report comment is sent in realtime

Create two accounts and a chat between them. Open a new NewDot window in a new Chrome session (do not use incognito mode).
Send messages back and forth from each account to the other in a DM chat
Verify they appear in realtime

Testing a report comment browser notification appears when the chat is not active (web)

Create two accounts and a chat between them. Open a new NewDot window in a new Chrome session (do not use incognito mode).
Send message from Account A to Account B while Account B is focused on another chat
Verify a browser notification appears for Account B

Testing a report comment is queued while offline and sent when online again

Create two accounts and a chat between them. Open a new NewDot window in a new Chrome session (do not use incognito mode).
Send message from Account A to Account B while Account A is "offline"
Verify Account A sees the message in a grey "pending" state
Bring Account A back online
Verify the message darkens to indicate it was created.
Verify Account B can also see the message now

Add an attachment

Create two accounts and a chat between them. Open a new NewDot window in a new Chrome session (do not use incognito mode).
Add an attachment to the chat between these two users.
Verify the attachment appears in realtime for the other user (Note: it may take some time for it to upload - this is normal).

Add an attachment (offline)

Create two accounts and a chat between them. Open a new NewDot window in a new Chrome session (do not use incognito mode).
Add an attachment to the chat between these two users while offline.
Verify the attachment appears greyed to indicate that it is pending.
Bring the user online.
Verify the attachment darkens to indicate it is created
Verify the attachment appears in realtime for the other user

Add attachment with text

Create two accounts and a chat between them. Open a new NewDot window in a new Chrome session (do not use incognito mode).
Add an attachment to the chat between these two users while there is text in the composer.
Verify the attachment and text appears in realtime for the other user.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Luke9389

Beautiful. Just nabs and clarifying questions

src/components/InlineSystemMessage.js

src/libs/actions/Report.js

marcaaron · 2022-06-15T20:54:26Z

This is still on HOLD, but I think ready for some reviews.

marcaaron · 2022-06-27T16:07:13Z

Putting a HOLD on this one while we work out some things in https://github.com/Expensify/Web-Expensify/pull/34086

marcaaron · 2022-06-30T23:07:54Z

This is still on hold pending the deploy of https://github.com/Expensify/Web-Expensify/pull/34086

However, we will need to merge this relatively quickly one that code hits production so I think we should start getting some reviews in.

marcaaron · 2022-07-05T16:18:53Z

Taking this off hold as the linked Web-Expensify PR is on staging.

marcaaron · 2022-07-05T16:19:13Z

We should only merge this when it's on production though.

marcaaron · 2022-07-05T17:40:35Z

Testing well for me on all platforms.

marcaaron · 2022-07-05T17:48:24Z

Just a reminder, this very urgently needs to be merged and deployed as soon as the Web-Expensify PR goes to prod. For that reason I am also CP-ing to staging.

github-actions · 2022-07-05T17:48:39Z

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

yuwenmemon

LGTM! 🎉

arosiclair

Looks solid 👍🏽

PauloGasparSv

LGTM!
Merging it already since it has to go fast

…Action2 Replace Report_AddComment with AddComment (Part 2) (cherry picked from commit 258c268)

…9350 🍒 Cherry pick PR #9350 to staging 🍒

bondydaa · 2022-07-05T23:18:31Z

src/libs/DateUtils.js

+function canUpdateTimezone() {
+    return lastUpdatedTimezoneTime.isBefore(moment().subtract(5, 'minutes'));
+}
+


I know this is just a refactor but why do we try to update every 5 minutes? feels like way too often, it probably only matters when a user is traveling but even then I don't really need my timezone updated every step of the way, just like when I arrive to my destination.

actually thinking about this more (i know this is all one comment but time passed between the first paragraph and this one)... why put a time limit on it at all? why not just check if the current timezone is different than the one in onyx and only attempt to set it if they are different?

That could work. I'm not sure why we chose every 5 minutes or whether it would be easier to check if the timezone has changed. I think there is some kind of cost associated with trying to "guess" the timezone and maybe that's why this is throttled, but not really focused on improving this particular feature.

bondydaa · 2022-07-06T00:41:21Z

src/libs/actions/Report.js

+        DateUtils.setTimezoneUpdated();
+    }
+
+    API.write(isAttachment ? 'AddAttachment' : 'AddComment', parameters, {


I think here I might have preferred a separate variable like

const command = isAttachment ? 'AddAttachment' : 'AddComment'; API.write(command, parameters, ...);

or maybe even getting rid of isAttachment and renaming that command at the top of the method and instead forking logic if command is AddAttachment or AddComment though that might end up being way too verbose 🤷

eh i guess looking back we use isAttachment inside of the onxyData and a few other places so maybe this is the best route the way it's written.

Don't really have strong feelings one way or the other on this.

bondydaa · 2022-07-06T00:46:24Z

tests/README.md

@@ -69,7 +69,7 @@ describe('actions/Report', () => {
                // the comment we left and it will be in a loading state because
                // it's an "optimistic comment"
                expect(action.message[0].text).toBe('Hello!');
-                expect(action.loading).toBe(true);
+                expect(action.isPending).toBe(true);


shouldn't this be isLoading?

yes good catch.

kavimuru · 2022-07-07T19:55:12Z

@marcaaron We never get notification in mweb.
Section 2:

Testing a report comment browser notification appears when the chat is not active (web)

Create two accounts and a chat between them. Open a new NewDot window in a new Chrome session (do not use incognito mode).

Send message from Account A to Account B while Account B is focused on another chat

Verify a browser notification appears for Account B

So step 4, No notification received in mWeb environment.

roryabraham · 2022-07-07T20:06:26Z

We never get notification in mweb

Correct me if I'm wrong @marcaaron, but I believe that's expected and maybe depends on the device/browser. I'm an iOS user and never get notifications from mobile websites like I do on my laptop.

marcaaron · 2022-07-07T20:10:19Z

Yeah, I have never seen a mobile web browser notification ever.

yuwenmemon · 2022-07-07T20:37:17Z

Yeah @kavimuru that isn't a blocker 👍

OSBotify · 2022-07-08T04:45:53Z

🚀 Deployed to production by @roryabraham in version: 1.1.79-17 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

marcaaron added 2 commits June 7, 2022 13:57

Add SystemInlineMessage and remove error handlers

aac44cb

Fix up optimisticReportActions

ae0e541

marcaaron self-assigned this Jun 8, 2022

marcaaron added 7 commits June 7, 2022 14:07

revert updateReportWithNewAction change for now

984f41e

use const

4b4bceb

use Onyx.update() for Airship

0ea7e2d

Fix jest tests

5759035

Fix styles

05d540d

Update everything to use isPending instead of loading

631932f

remove unused spacing

665b93b

Luke9389 reviewed Jun 9, 2022

View reviewed changes

src/components/InlineSystemMessage.js Show resolved Hide resolved

src/libs/actions/Report.js Outdated Show resolved Hide resolved

src/libs/actions/Report.js Show resolved Hide resolved

marcaaron added 2 commits June 9, 2022 12:06

fix conflicts

bf90793

remove unused stuff

33eb904

marcaaron mentioned this pull request Jun 9, 2022

Fix last message text in LHN includes text and no space above quoted text #8814

Merged

89 tasks

marcaaron added 7 commits June 10, 2022 10:57

Remove throttled timezone update call

d798541

Update both my details and personal details

a7174be

stringify

7ba89c1

Just keep CreateComment and CreateAttachment separate for now

950ea10

Keep the name save the refactor for later

a6ad485

Fix test

99306f1

fix conflicts

de49280

marcaaron changed the title ~~[HOLD] [WIP] Replace Report_AddComment with AddComment (Part 2)~~ [HOLD] Replace Report_AddComment with AddComment (Part 2) Jun 15, 2022

marcaaron changed the title ~~[HOLD] Replace Report_AddComment with AddComment (Part 2)~~ [HOLD] Replace Report_AddComment with CreateComment (Part 2) Jun 15, 2022

marcaaron marked this pull request as ready for review June 15, 2022 20:53

marcaaron requested a review from a team as a code owner June 15, 2022 20:53

marcaaron changed the title ~~[HOLD] Replace Report_AddComment with CreateComment (Part 2)~~ [HOLD Web-E 33951] Replace Report_AddComment with CreateComment (Part 2) Jun 15, 2022

melvin-bot bot requested review from arosiclair and removed request for a team June 15, 2022 20:54

marcaaron requested a review from yuwenmemon June 15, 2022 20:54

marcaaron changed the title ~~Replace Report_AddComment with CreateComment (Part 2)~~ Replace Report_AddComment with AddComment (Part 2) Jun 23, 2022

marcaaron changed the title ~~Replace Report_AddComment with AddComment (Part 2)~~ [HOLD] Replace Report_AddComment with AddComment (Part 2) Jun 27, 2022

marcaaron mentioned this pull request Jun 29, 2022

[$250] Web - Attachment - Attachment tag displayed in LHN after refresh #9539

Closed

marcaaron changed the title ~~[HOLD] Replace Report_AddComment with AddComment (Part 2)~~ Replace Report_AddComment with AddComment (Part 2) Jul 5, 2022

marcaaron mentioned this pull request Jul 5, 2022

Deploy Checklist: New Expensify 2022-06-28 #9586

Closed

71 tasks

marcaaron added the CP Staging label Jul 5, 2022

yuwenmemon approved these changes Jul 5, 2022

View reviewed changes

arosiclair approved these changes Jul 5, 2022

View reviewed changes

PauloGasparSv approved these changes Jul 5, 2022

View reviewed changes

PauloGasparSv merged commit 258c268 into main Jul 5, 2022

PauloGasparSv deleted the marcaaron-updateReportWithNewAction2 branch July 5, 2022 21:17

OSBotify pushed a commit that referenced this pull request Jul 5, 2022

Merge pull request #9350 from Expensify/marcaaron-updateReportWithNew…

ea795fa

…Action2 Replace Report_AddComment with AddComment (Part 2) (cherry picked from commit 258c268)

OSBotify mentioned this pull request Jul 5, 2022

🍒 Cherry pick PR #9350 to staging 🍒 #9720

Merged

OSBotify added a commit that referenced this pull request Jul 5, 2022

Merge pull request #9720 from Expensify/OSBotify-cherry-pick-staging-…

c272b4c

…9350 🍒 Cherry pick PR #9350 to staging 🍒

bondydaa reviewed Jul 6, 2022

View reviewed changes

kbecciv mentioned this pull request Jul 7, 2022

Attachment - Text is duplicated when sent message with attachment while offline #9766

Closed

OSBotify mentioned this pull request Jul 8, 2022

Deploy Checklist: New Expensify 2022-07-07 #9781

Closed

OSBotify mentioned this pull request Aug 16, 2022

Deploy Checklist: New Expensify 2022-08-04 #10253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Report_AddComment with AddComment (Part 2) #9350

Replace Report_AddComment with AddComment (Part 2) #9350

marcaaron commented Jun 8, 2022 •

edited

Loading

Luke9389 left a comment

marcaaron commented Jun 15, 2022

marcaaron commented Jun 27, 2022

marcaaron commented Jun 30, 2022

marcaaron commented Jul 5, 2022

marcaaron commented Jul 5, 2022

marcaaron commented Jul 5, 2022

marcaaron commented Jul 5, 2022

github-actions bot commented Jul 5, 2022

yuwenmemon left a comment

arosiclair left a comment

PauloGasparSv left a comment

bondydaa Jul 5, 2022

marcaaron Jul 6, 2022

bondydaa Jul 6, 2022

bondydaa Jul 6, 2022

marcaaron Jul 6, 2022

bondydaa Jul 6, 2022

marcaaron Jul 6, 2022

kavimuru commented Jul 7, 2022

roryabraham commented Jul 7, 2022

marcaaron commented Jul 7, 2022

yuwenmemon commented Jul 7, 2022

OSBotify commented Jul 8, 2022

Replace Report_AddComment with AddComment (Part 2) #9350

Replace Report_AddComment with AddComment (Part 2) #9350

Conversation

marcaaron commented Jun 8, 2022 • edited Loading

Details

Fixed Issues (Related to

Tests

QA Steps

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Luke9389 left a comment

Choose a reason for hiding this comment

marcaaron commented Jun 15, 2022

marcaaron commented Jun 27, 2022

marcaaron commented Jun 30, 2022

marcaaron commented Jul 5, 2022

marcaaron commented Jul 5, 2022

marcaaron commented Jul 5, 2022

marcaaron commented Jul 5, 2022

github-actions bot commented Jul 5, 2022

yuwenmemon left a comment

Choose a reason for hiding this comment

arosiclair left a comment

Choose a reason for hiding this comment

PauloGasparSv left a comment

Choose a reason for hiding this comment

bondydaa Jul 5, 2022

Choose a reason for hiding this comment

marcaaron Jul 6, 2022

Choose a reason for hiding this comment

bondydaa Jul 6, 2022

Choose a reason for hiding this comment

bondydaa Jul 6, 2022

Choose a reason for hiding this comment

marcaaron Jul 6, 2022

Choose a reason for hiding this comment

bondydaa Jul 6, 2022

Choose a reason for hiding this comment

marcaaron Jul 6, 2022

Choose a reason for hiding this comment

kavimuru commented Jul 7, 2022

roryabraham commented Jul 7, 2022

marcaaron commented Jul 7, 2022

yuwenmemon commented Jul 7, 2022

OSBotify commented Jul 8, 2022

marcaaron commented Jun 8, 2022 •

edited

Loading